New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add URL checks to Doctor #5760
Add URL checks to Doctor #5760
Conversation
lib/jekyll/commands/doctor.rb
Outdated
@@ -35,7 +37,8 @@ def healthy?(site) | |||
fsnotify_buggy?(site), | |||
!deprecated_relative_permalinks(site), | |||
!conflicting_urls(site), | |||
!urls_only_differ_by_case(site) | |||
!urls_only_differ_by_case(site), | |||
proper_site_url?(site) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If only Rubocop allowed Style/TrailingCommaInLiteral
😡
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pathawks what prevents us from adding Style/TrailingCommaInLiteral: { EnforcedStyleForMultiline: consistent_comma }
to .rubocop.yml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DirtyF Laziness 😝
I should probably add a check for |
/cc: @jekyll/core @jekyll/ecosystem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Not quite sure about the CI failures there, but 👍 on the content of the PR.
fd13545
to
c123b10
Compare
Fixes #5718 |
CI failures have been fixed 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So now you've got to set site.url
? Before it was not mandatory and was automatically set in development mode or by GitHub Pages.
It's not mandatory, but since leaving it blank can cause problems with plugins or themes, users could at least be made aware of the situation. Note that this only runs when a user runs If a user is intentionally leaving |
Hm, maybe just don't run the check then? |
@pathawks so should we break those tests: one that checks if |
url = site.config["url"] | ||
[ | ||
url_exists?(url), | ||
url_valid?(url), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of moving url_exists?
out of proper_site_url?
in a separate single test to address Parker's concern, so we test first if site.url
exists before launching proper_site_url
. But as I suck at logic and programming, this is maybe a terrible idea :)
If what you are proposing does that, it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow.
We really want site.url
to be set, either manually by the user or automatically by something like pages-gem
. If site.url
is blank, it is likely to cause problems and we should warn the user about this.
If a user has some special reason to use a blank site.url
then they will understand that this warning does not apply to them.
On the other hand, if a user has simply neglected to set site.url
and now things aren't working as expected, this check should alert them to the issue and suggest setting site.url
. That is the entire point of this PR.
I'm probably not understanding your concerns; can you try to explain to me again now that I've explained my intent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get you perfectly and as it's impossible to tell if a blank site.url
is intentional or not, you prefer to warn in both cases.
Maybe just make the messages more clear then?
bundle exec jekyll doctor
Warning: You didn't set an URL in the config file, you may encounter problems with some plugins.
bundle exec jekyll doctor
Warning: The site URL does not seem to be valid, check the value of `url` in your config file.
bundle exec jekyll doctor
Warning: Your site URL does not seem to be absolute, check the value of `url` in your config file.
@pathawks BTW what do you think about adding a "check your configuration" section with |
@DirtyF Good idea. We should also add a request to the PR template that users post the output of |
For an example of what I hope to accomplish with this: jekyll/jekyll-sitemap#171 |
@jekyllbot: merge +minor Thanks @pathawks ❤️ |
If a site does not provide an absolute URL in
site.url
, all sorts of wonky behavior can creep up, especially as sites rely more heavily on themes plugins that assume standard behavior.To this end, I would like to add some simple URL checks to
jekyll doctor
to make sure that things are in good working order.